Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Deserialization #3

Conversation

joaoantoniocardoso
Copy link
Member

No description provided.

@joaoantoniocardoso joaoantoniocardoso force-pushed the add_simple_deserialization branch from 045bd81 to e40fe6d Compare January 15, 2023 03:53
@patrickelectric
Copy link
Member

@joaoantoniocardoso can you rebase your work ?

@@ -5,7 +5,7 @@ use ping_rs::{common, PingMessagePack};
fn test_simple_serialization() {
let general_request =
common_messages::GeneralRequest(common::GeneralRequestStruct { requested_id: 5 });
let message = PingMessagePack::from(general_request);
let message = PingMessagePack::from(&general_request);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears unnecessary on the current version

src/lib.rs Show resolved Hide resolved

fn try_from(buffer: &Vec<u8>) -> Result<Self, Self::Error> {
// Parse start1 and start2
if !((buffer[0] == b'B') && (buffer[1] == b'R')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should have a function and variables to compare the header and save B and R.


fn try_from(buffer: &Vec<u8>) -> Result<Self, Self::Error> {
// Parse start1 and start2
if !((buffer[0] == b'B') && (buffer[1] == b'R')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a generic deserializer that stores the state of the parser. If we receive B, than R, than 3 more bytes, until the message is complete.

let data_type = field.typ.to_rust();
let data_size = field.typ.to_size();
let field_token = quote! {
#name: #data_type::from_le_bytes(payload[#b..#b + #data_size].try_into().expect("Wrong slice length")),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can check the initial size with payload.len(), try_into and everything else will be unnecessary,

let data_type = field.typ.to_rust();
let data_size = field.typ.to_size();
let field_token = quote! {
#name: #data_type::from_le_bytes(payload[#b..#b + #data_size].try_into().expect("Wrong slice length")),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should do #b + #data_size before putting on the template for better readability of the generated code.

// Get the package data
let payload_length = u16::from_le_bytes([buffer[2], buffer[3]]);
let message_id = u16::from_le_bytes([buffer[4], buffer[5]]);
let _src_device_id = buffer[6];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should think how to handle it in a better way, maybe returning two structures, one that encapsulate the message and another one for the header or everything else, maybe a complete message.

@patrickelectric
Copy link
Member

@joaoantoniocardoso any update on that ?

@patrickelectric patrickelectric force-pushed the add_simple_deserialization branch from e40fe6d to 81a9a98 Compare December 24, 2023 11:40
@patrickelectric patrickelectric marked this pull request as ready for review December 24, 2023 11:41
Copy link
Member

@patrickelectric patrickelectric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll continue working over it

@patrickelectric patrickelectric merged commit 9a201fd into bluerobotics:master Dec 24, 2023
1 of 2 checks passed
@joaoantoniocardoso joaoantoniocardoso deleted the add_simple_deserialization branch December 24, 2023 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants